Skip to content

Clear handler cache - #193

Merged
ritch merged 1 commit into
strongloop:masterfrom
globocom:clear-handler-cache2
Feb 18, 2014
Merged

Clear handler cache#193
ritch merged 1 commit into
strongloop:masterfrom
globocom:clear-handler-cache2

Conversation

@gcirne

@gcirne gcirne commented Feb 18, 2014

Copy link
Copy Markdown
Contributor

(For some background to this PR, please see #187)

This is the simplest possible way I can think of clearing the handler cache after registering a new model. I've opened this PR as a starting point so that you guys can help point me in the right direction.

@bajtos, @rmg, @ritch what do you think?

@slnode

slnode commented Feb 18, 2014

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@rmg

rmg commented Feb 18, 2014

Copy link
Copy Markdown
Member

ok to test

@slnode

slnode commented Feb 18, 2014

Copy link
Copy Markdown

Refer to this link for build results: http://ci.strongloop.com/job/loopback/950/

@ritch

ritch commented Feb 18, 2014

Copy link
Copy Markdown
Member

Approach and code look good to me.

ritch added a commit that referenced this pull request Feb 18, 2014
@ritch
ritch merged commit edd16b9 into strongloop:master Feb 18, 2014
@gcirne

gcirne commented Feb 19, 2014

Copy link
Copy Markdown
Contributor Author

Thanks @ritch!

Comment thread test/app.test.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rewrite this test to describe the behaviour as observed by a LB user - i.e. Color is available via REST - instead of describing the implementation details?

Something along these lines:

app.use(loopback.rest());
request(app).get('/colors').expect(404, function(err, res) {
  if (err) return done(err);
  var Color = db.createModel('color', {name: String});
  app.model(Color);
  request(app).get('/colors').expect(200, done);
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I have not realised the patch had already landed. Never mind, this is not critical, the test can stay as it is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could rewrite the test, no problem. Then maybe you guys could merge it
again.

What do you think?

On Wednesday, February 19, 2014, Miroslav Bajtoš notifications@github.com
wrote:

In test/app.test.js:

@@ -29,6 +29,14 @@ describe('app', function() {
expect(app.remotes().exports).to.eql({ color: Color });
});

  • it('clears handler cache', function() {
  •  var originalRestHandler = app.handler('rest');
    
  •  var Color = db.createModel('color', {name: String});
    
  •  app.model(Color);
    
  •  var newRestHandler = app.handler('rest');
    
  •  expect(originalRestHandler).to.not.equal(newRestHandler);
    
  • });

Hmm, I have not realised the patch had already landed. Never mind, this is
not critical, the test can stay as it is.


Reply to this email directly or view it on GitHubhttps://github.com//pull/193/files#r9861989
.

Guilherme Machado Cirne
gcirne@gmail.com

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcirne Don't worry about that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos has a good point. We are specifying (via the test) that calling app.model() results in a different handler. Ideally we would be able to rewrite the handler mechanism and this test should still pass.

@gcirne - feel free to submit a PR that rewrites the test. Not a priority IMO though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants